Fix misleading tracer cache invalidation comment and simplify logic#97
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Durable Task JS tracing layer, primarily around tracer caching and trace-context injection, and adjusts tracing conventions used by spans and tests.
Changes:
- Introduces a cached tracer lookup via
getTracingContext()and updates comments around why caching is safe. - Adds
createPbTraceContextFromSpan()to build protobufTraceContextdirectly from an OTELSpan, and switches injection sites to use it. - Updates timer span naming and adjusts expected tracing attributes/names in tests to match the updated conventions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/durabletask-js/src/tracing/trace-helper.ts | Adds cached tracer/context helper, uses it throughout span creation, and switches proto trace-context injection to createPbTraceContextFromSpan; also changes some durabletask.type values. |
| packages/durabletask-js/src/tracing/trace-context-utils.ts | Adds createPbTraceContextFromSpan() helper for protobuf trace context creation directly from a span context. |
| packages/durabletask-js/src/tracing/constants.ts | Updates timer span name format to orchestration:{orchName}:timer. |
| packages/durabletask-js/src/tracing/index.ts | Re-exports createPbTraceContextFromSpan. |
| packages/durabletask-js/test/tracing.spec.ts | Updates expected timer span naming and task-type attributes; adds coverage for createPbTraceContextFromSpan. |
| packages/durabletask-js/test/worker-tracing.spec.ts | Updates sub-orchestration scheduling span expectations to match updated durabletask.type conventions and span kind filtering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
YunchuWang
reviewed
Feb 9, 2026
YunchuWang
approved these changes
Feb 9, 2026
Member
|
Member
|
Contributor
…atePbTraceContextFromSpan signature - Revert setSpanError and setSpanOk to call getOtelApi() instead of getTracingContext(), since they only need SpanStatusCode (no tracer) - Widen createPbTraceContextFromSpan parameter type to Span | undefined | null for consistency with extractTraceparentFromSpan
YunchuWang
approved these changes
Feb 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed?
_cachedOtelvariable fromtrace-helper.tsif (!_cachedTracer || _cachedOtel !== otel)toif (!_cachedTracer)Why is this change needed?
The previous comment claimed "The cache is invalidated if the global tracer provider changes (e.g., in tests)" but the code only compared the OTEL API module reference (
_cachedOtel !== otel). SincegetOtelApi()always returns the same module object (due to Node'srequire()cache and internal_otelApiLoadedguard), this condition was never true after the first call — making_cachedOteldead code and the comment misleading.The tracer caching is actually safe because the OTEL JS SDK's
trace.getTracer()returns a proxy tracer that dynamically delegates to the current global tracer provider. Provider swaps (e.g., viasetGlobalTracerProvider()in tests) are handled transparently without needing to recreate the tracer. The new comment explains this correctly.Issues / work items
Project checklist
tracing.spec.ts,worker-tracing.spec.ts) continue to passAI-assisted code disclosure (required)
Was an AI tool used? (select one)
If AI was used:
packages/durabletask-js/src/tracing/trace-helper.ts— comment rewrite and dead code removalAI verification (required if AI was used):
Testing
Automated tests
Manual validation (only if runtime/behavior changed)
Notes for reviewers
trace.getTracer()returns a proxy tracer that delegates to the current global provider at call time. This means the cached tracer automatically picks up provider changes without invalidation, making the previous_cachedOtelcheck unnecessary.